-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpf: Inline assembly for packet context access #25336
Conversation
The BPF unit tests currently pass the verifier just fine. "If it ain't broke, don't fix it." Oh but of course, that would be too easy. In a future commit, we will implement BPF-based IPv6 masquerading, for which we face an issue with the verifier: when accessing the context, LLVM sometimes generates 32-bit assignments, leading to the verifier being unable to track the packet pointers. We have a fix for that: we can modify the way we do packet context accesses, preventing LLVM to perform optimisations by using inline assembly. But the "asm volatile()" calls come at a cost: if called multiple times ctx_data() and ctx_data_end() will no longer be optimised out, and instead will reset the tracking of packet pointers. To fix the fix for what's not broken yet, in other words: in preparation for inline assembly context access, make sure we do not call ctx_data() and ctx_data_end() more than once in the BPF unit tests, unless the calls are separated by a call to a helper function that resets tracking anyway. Suggested-by: Yonghong Song <yhs@fb.com> Suggested-by: Maxim Mikityanskiy <maxim@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Since ctx->{data,data_end,data_meta} are 32-bits fields, LLVM sometimes generates 32-bit assignments for those pointers. This leads to verifier errors as the verifier is unable to track the packet pointers through the 32-bit assignments, as show below. ; return (void *)(unsigned long)ctx->data; 2: (61) r9 = *(u32 *)(r7 +76) ; R7_w=ctx(id=0,off=0,imm=0) R9_w=pkt(id=0,off=0,r=0,imm=0) ; return (void *)(unsigned long)ctx->data; 3: (bc) w6 = w9 ; R6_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R9_w=pkt(id=0,off=0,r=0,imm=0) ; if (data + tot_len > data_end) 4: (bf) r2 = r6 ; R2_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff)) 5: (07) r2 += 54 ; R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff)) ; if (data + tot_len > data_end) 6: (2d) if r2 > r1 goto pc+466 ; R1_w=pkt_end(id=0,off=0,imm=0) R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff)) ; tmp = a->d1 - b->d1; 7: (71) r2 = *(u8 *)(r6 +22) R6 invalid mem access 'inv' As a workaround, Yonghong Song suggested to read those fields using asm bytecode. With that trick, LLVM is unaware that the original field is actually on 32 bits and can't perform the above "optimization". This workaround is needed only now because a subsequent commit introduces some BPF C code that causes this bytecode to be generated (snat_v6_needed). [ Quentin: Add #undef DEFINE_FUNC_CTX_POINTER. ] Related: https://lore.kernel.org/bpf/c5a76b4d-abed-51f6-bf16-040eb0baf290@fb.com/T/#m933626f7404562a6e98af78a8f44c30977681ffa Suggested-by: Yonghong Song <yhs@fb.com> Signed-off-by: Paul Chaignon <paul@cilium.io>
/test |
/test-1.26-net-next |
Checkpatch report is a formatting nit that we can ignore in this case - we prefer to keep related definitions close together in this commit rather than adding newlines between them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good to me! I'm glad we won't have verifier issues with packet pointers anymore.
We can probably revert the workaround as a follow-up? Also, I believe there are a few revalidate_data calls in the code that can be ditched after your patch.
--
No need to change formatting in the code, but I wonder why checkpatch complains on this:
Warning: WARNING:LINE_CONTINUATIONS: Avoid unnecessary line continuations
#86: FILE: bpf/include/bpf/ctx/skb.h:72:
+ */ \
Or does it actually mean the previous lines of the multiline comment, not the last one?
Yes good call, I can look into that next.
The comment in checkpatch.pl says: # check for line continuations outside of #defines, preprocessor #, and asm so my guess is that between the |
Given that
ctx->
{data
,data_end
,data_meta
} are 32-bits fields, LLVM sometimes generates 32-bit assignments for those pointers. This leads to verifier errors as the verifier is unable to track the packet pointers through the 32-bit assignments, as show below.As a workaround, Yonghong Song suggested to read those fields using asm bytecode. With that trick, LLVM is unaware that the original field is actually on 32 bits and can't perform the above "optimisation".
This workaround is needed only now because code for IPv6 masquerading, coming in a future PR, will cause this bytecode to be generated (snat_v6_needed).
Preliminary to this change, we also need some adjustments in the BPF unit tests, to avoid resetting range tracking for packet pointers by fetching them multiple times (these multiple fetches were previously optimised out by the compiler, but also drop this optimisation by switching to inline assembly).